chore: adding upgrade handler for 09-localhost removal #1671

damiannolan · 2022-07-07T11:41:55Z

Description

Adding upgrade handler for pruning of 09-localhost client and consensus states
Adding migration docs

closes: #1201

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

damiannolan · 2022-07-07T11:44:25Z

I think this PR also requires some migration docs added to instruct devs to wire up the upgrade handler.
I will add some!

I've added a new directory upgrades/v5 under core. I am open to suggestions on moving this to somewhere else but I think this is a nice pattern to use for upgrade handlers for ibc core logic. Inspired from osmosis here

…localhost

docs/migrations/v4-to-v5.md

…localhost

crodriguezvega

Maybe we could call the folder migrations? Not very strongly opposed to upgrades anyway, but thought that migrations is how the folder is called in docs and we could also have migrations that not necessarily need to run on the upgrade to a major version. Well, just an idea!

docs/migrations/v4-to-v5.md

colin-axner

Looks great :)

Might be nice to add information to the localhost removal changelog entry that this function can be used to remove existing localhost implementations

docs/migrations/v4-to-v5.md

modules/core/upgrades/v5/upgrades.go

…cosmos/ibc-go into damian/1201-migrations-for-09-localhost

damiannolan · 2022-07-11T16:21:35Z

Maybe we could call the folder migrations? Not very strongly opposed to upgrades anyway, but thought that migrations is how the folder is called in docs and we could also have migrations that not necessarily need to run on the upgrade to a major version. Well, just an idea!

Which approach should we take with naming? I chose upgrades as it was an "upgrade handler" in the context of the sdk that was being added, as opposed to the legacy genesis migration approach but I'm not really fussy. We could rename the docs/migrations directory to docs/upgrades or we go with package migrations in the go code. I guess its best to have some consistency. My personal vote is for package upgrades with a vX sub package, but I'm happy to go with the consensus.

colin-axner · 2022-07-12T10:07:53Z

Which approach should we take with naming?

I like migrations. Typically this functionality would be run by SDK's RunMigrations function. It isn't only because most chains don't have a localhost thus making the extra computation unnecessary. I also want to avoid confusion with client/connection/channel upgrades

damiannolan · 2022-07-12T10:14:58Z

Which approach should we take with naming?

I like migrations. Typically this functionality would be run by SDK's RunMigrations function. It isn't only because most chains don't have a localhost thus making the extra computation unnecessary. I also want to avoid confusion with client/connection/channel upgrades

Sounds good, that's a compelling enough argument for me! I'll update this today with the naming changes throughout 👍

…localhost

crodriguezvega

Just a small nit..
Nice work, @damiannolan!

CHANGELOG.md

charleenfei

nice :)

charleenfei · 2022-07-15T11:41:58Z

modules/core/migrations/v5/migrations.go

+const Localhost string = "09-localhost"
+
+// MigrateToV5 prunes the 09-Localhost client and associated consensus states from the ibc store
+func MigrateToV5(ctx sdk.Context, clientKeeper clientkeeper.Keeper) {


nit: we could potentially rename this function to make it more explicit as to what it does? (something like Migrate_RemoveLocalHostClient)

charleenfei · 2022-07-15T11:45:57Z

docs/migrations/v4-to-v5.md

+// ...
+
+app.UpgradeKeeper.SetUpgradeHandler(
+    upgradeName,


nit: upgradeName can be MigrateToV5 or MigrateLocalHostClient?

Co-authored-by: Carlos Rodriguez <carlos@interchain.io>

* adding localhost migration code with tests * updating test cases * renaming test func * updating migration docs and moving to v4-to-v5.md * fixing indentation of code snippet * Update docs/migrations/v4-to-v5.md * adding sample query check for localhost client to docs * updating changelog to provide info on localhost upgrade handler * updating migrations docs with nit * renaming upgrades to migrations * updating function namings, tests and docs * Update CHANGELOG.md Co-authored-by: Carlos Rodriguez <carlos@interchain.io> Co-authored-by: Carlos Rodriguez <carlos@interchain.io>

damiannolan added 2 commits July 7, 2022 11:06

adding localhost migration code with tests

b47d85b

updating test cases

621e3b3

damiannolan requested review from colin-axner, fedekunze and AdityaSripal as code owners July 7, 2022 11:41

Merge branch '02-client-refactor' into damian/1201-migrations-for-09-…

a6fb9fe

…localhost

damiannolan requested review from charleenfei, chatton and crodriguezvega July 7, 2022 11:44

damiannolan added 2 commits July 7, 2022 12:47

renaming test func

0555fa1

updating migration docs and moving to v4-to-v5.md

2fa1c3b

damiannolan requested a review from seantking as a code owner July 7, 2022 12:51

damiannolan commented Jul 7, 2022

View reviewed changes

docs/migrations/v4-to-v5.md Outdated Show resolved Hide resolved

fixing indentation of code snippet

b773383

damiannolan commented Jul 7, 2022

View reviewed changes

docs/migrations/v4-to-v5.md Outdated Show resolved Hide resolved

damiannolan added 2 commits July 7, 2022 15:05

Update docs/migrations/v4-to-v5.md

53a813e

Merge branch '02-client-refactor' into damian/1201-migrations-for-09-…

b940630

…localhost

crodriguezvega reviewed Jul 8, 2022

View reviewed changes

docs/migrations/v4-to-v5.md Outdated Show resolved Hide resolved

colin-axner approved these changes Jul 11, 2022

View reviewed changes

docs/migrations/v4-to-v5.md Show resolved Hide resolved

modules/core/upgrades/v5/upgrades.go Outdated Show resolved Hide resolved

damiannolan added 4 commits July 11, 2022 16:49

adding sample query check for localhost client to docs

08c520e

updating changelog to provide info on localhost upgrade handler

bdec3f0

Merge branch 'damian/1201-migrations-for-09-localhost' of github.com:…

fac2a66

…cosmos/ibc-go into damian/1201-migrations-for-09-localhost

updating migrations docs with nit

5a3a78b

damiannolan added 3 commits July 12, 2022 11:30

renaming upgrades to migrations

bc59d85

updating function namings, tests and docs

755a66b

Merge branch '02-client-refactor' into damian/1201-migrations-for-09-…

2d04900

…localhost

crodriguezvega approved these changes Jul 15, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

charleenfei approved these changes Jul 15, 2022

View reviewed changes

Update CHANGELOG.md

7b08360

Co-authored-by: Carlos Rodriguez <carlos@interchain.io>

damiannolan merged commit 04df7cd into 02-client-refactor Jul 15, 2022

damiannolan deleted the damian/1201-migrations-for-09-localhost branch July 15, 2022 21:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: adding upgrade handler for 09-localhost removal #1671

chore: adding upgrade handler for 09-localhost removal #1671

damiannolan commented Jul 7, 2022 •

edited

Loading

damiannolan commented Jul 7, 2022

crodriguezvega left a comment

colin-axner left a comment

damiannolan commented Jul 11, 2022

colin-axner commented Jul 12, 2022

damiannolan commented Jul 12, 2022

crodriguezvega left a comment

charleenfei left a comment

charleenfei Jul 15, 2022

charleenfei Jul 15, 2022

chore: adding upgrade handler for 09-localhost removal #1671

chore: adding upgrade handler for 09-localhost removal #1671

Conversation

damiannolan commented Jul 7, 2022 • edited Loading

Description

damiannolan commented Jul 7, 2022

crodriguezvega left a comment

Choose a reason for hiding this comment

colin-axner left a comment

Choose a reason for hiding this comment

damiannolan commented Jul 11, 2022

colin-axner commented Jul 12, 2022

damiannolan commented Jul 12, 2022

crodriguezvega left a comment

Choose a reason for hiding this comment

charleenfei left a comment

Choose a reason for hiding this comment

charleenfei Jul 15, 2022

Choose a reason for hiding this comment

charleenfei Jul 15, 2022

Choose a reason for hiding this comment

damiannolan commented Jul 7, 2022 •

edited

Loading